gh-127681: add overflow checks when using PyMem_Malloc#127686
Conversation
picnixz
changed the title
PyMem_Malloc
|
I've added comments for future readers so that they know that something does not need to be checked or not, but I can also remove them if you think it's overly verbose. |
picnixz
requested review from
FFY00,
gpshead,
markshannon,
methane and
rhettinger
as code owners
serhiy-storchaka
left a comment
serhiy-storchaka
left a comment
There was a problem hiding this comment.
I reviewed, and found only one site where an integer overflow may be possible. In all other cases it is not possible.
| if (prefix) { | ||
| size_t prefix_len = strlen(prefix); | ||
| if (len > PY_SIZE_MAX - prefix_len) { | ||
| goto oom; | ||
| } | ||
| len += prefix_len; | ||
| } | ||
| if (len == PY_SIZE_MAX) { | ||
| goto oom; | ||
| } |
There was a problem hiding this comment.
Do you have an example of overflow? AFAIK, prefix is smaller than 1030 bytes, and suffix also is a short string.
| if (prefix) { | ||
| size_t l = strlen(prefix); | ||
| if ((size_t)prefix > (size_t)PY_SSIZE_T_MAX - l) { | ||
| goto oom; | ||
| } | ||
| prefix_len += l; | ||
| } |
There was a problem hiding this comment.
prefix is always "&" here.
| goto error; | ||
| stginfo->ndim = iteminfo->ndim + 1; | ||
| stginfo->shape = PyMem_Malloc(sizeof(Py_ssize_t) * stginfo->ndim); | ||
| stginfo->shape = PyMem_New(Py_ssize_t, stginfo->ndim); |
There was a problem hiding this comment.
This never overflows. ndim < 32.
| 33% of the creation time for c_int(). | ||
| */ | ||
| obj->b_ptr = (char *)PyMem_Malloc(info->size); | ||
| obj->b_ptr = PyMem_New(char, info->size); |
| slicelen); | ||
| } | ||
| dest = (char *)PyMem_Malloc(slicelen); | ||
| dest = PyMem_New(char, slicelen); |
| } | ||
|
|
||
| c_fds_to_keep = PyMem_Malloc(fds_to_keep_len * sizeof(int)); | ||
| c_fds_to_keep = PyMem_New(int, fds_to_keep_len); |
There was a problem hiding this comment.
It never overflows. fds_to_keep_len is the size of a tuple. sizeof(int) <= sizeof(PyObject *).
|
|
||
| /* check for overflow */ | ||
| if ((ncodes + 1) > ((size_t)PY_SSIZE_T_MAX / sizeof(formatcode))) { | ||
| if (ncodes > ((size_t)PY_SSIZE_T_MAX / sizeof(formatcode)) - 1) { |
There was a problem hiding this comment.
ncodes + 1 does not overflow. ncodes is not longer then the length of str/bytes.
|
|
||
| /* XXX(nnorwitz): need to check for overflow! */ | ||
| indices = (Py_ssize_t *)PyMem_Malloc(sizeof(Py_ssize_t)*view_src.ndim); | ||
| indices = PyMem_New(Py_ssize_t, view_src.ndim); |
There was a problem hiding this comment.
It does not overflow. ndim < 32.
|
|
||
| Py_ssize_t argcount = PyTuple_GET_SIZE(args); | ||
| if (argcount + 1 <= (Py_ssize_t)Py_ARRAY_LENGTH(small_stack)) { | ||
| if (argcount <= (Py_ssize_t)Py_ARRAY_LENGTH(small_stack) - 1) { |
There was a problem hiding this comment.
argcount + 1 does not overflow. argcount is the length of a tuple.
|
|
||
| garbage = (PyObject**) | ||
| PyMem_Malloc(slicelength*sizeof(PyObject*)); | ||
| garbage = PyMem_New(PyObject *, slicelength); |
There was a problem hiding this comment.
It does not overflow. slicelength is the length of a slice in existing list. The list already uses at least slicelength*sizeof(PyObject*) bytes.
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
|
So actually, there are parts that could be removed instead of fortified. I won't work on this one today but my question is: can I actually add the comments so that we know that there won't be someone else wondering "oh maybe we should check for this"? |
|
I don't like pollution of the code with trivial comments. They actually make reading the code more difficult -- you need to read and comprehend the comment instead of just skipping the presumably correct code. It is responsibility of the contributor to prove that they changes are necessary. The CPython code is mature enough, and most trivial bugs are fixed. So I suggest you to close this PR and open a new PR with only cases for which you can show at least theoretical possibility of integer overflow. |
2 participants
For now, this only adds checks in
ModulesandObjects.